Skip to content

fix: keep the status of synchronous responses on late client failures - #1608

Merged
jlouvel merged 3 commits into
3.0from
fix/netty-client-late-failure-status
Nov 1, 2026
Merged

jlouvel merged 3 commits into
3.0from
fix/netty-client-late-failure-status

Conversation

@jlouvel

@jlouvel jlouvel commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Refs #1601.

Cause

The FINE logs from #1602 captured a failure in CI. In SecurityTestCase, the client gets the 403 head and the first bytes of the server's HTML error page, so resource.get() returns. The test then calls resource.release(), which stops the ClientResource's own client while the rest of the body is still arriving. Closing the connection fails the in-flight call. Since the head was already received, dispatchResponseFailureCallback() then set the status to CONNECTOR_ERROR_COMMUNICATION (1001), even for synchronous requests. That's the same Response object the test reads afterwards, so its 403 turned into 1001.

.708  Received the response head 403 Forbidden
.708  Stopping the Netty client          (test calls release())
.708  Closed the connection
.709  The exchange failed: IOException "Connection closed by peer"

The passing runs show the same order, but there the end of the response arrives before the close.

Changes

  • NettyClientCall:
    • Synchronous requests: a failure after the head no longer changes the status. It reaches the response entity stream only, as an IOException on read.
    • Asynchronous requests: unchanged. The failure still sets 1001 and invokes the onResponse callback.
    • Empty bodies: after the head of a response with Content-Length: 0 (and no chunked encoding), a failure on the connection isn't reported as a response failure.
    • Completion order: when the response completes, the connection is released before the body stream ends. A synchronous caller resumes when the body ends, so its next request now always finds the connection back in the pool. Before, it could open a new connection, which made the empty-body test time out on CI (fixed in 62e72d6, after the rebase on 3.0).
  • NettyClientHandler and NettyClientHelper: stop() marks its connections before closing them. A call cut short by the stop now fails with IOException("Client stopped") instead of "Connection closed by peer". For HTTP/2 and HTTP/3 streams, the mark on the parent connection is checked too.
  • ClientLateFailureTestCase (new), with a raw socket server:
    • The server sends a 403 head and a first chunk, waits until the client stops, then sends the last chunk. The test checks that the 403 is kept, and that the entity stream fails with "Client stopped". Without the fix it fails 3 times out of 3, with the CI symptom: expected: <Forbidden (403)> but was: <Communication Error (1001)>.
    • Two Content-Length: 0 responses on one connection: both 403 with an empty entity, and the connection is reused.
  • changes.md: entry under Bugs fixed.

Http2CancellationTestCase still passes: it's asynchronous, so its callback and status still get 1001.

Validation

./mvnw -B -pl org.restlet -P default,test verify: 2217 tests pass, the coverage check passes, and SpotBugs still reports 8 findings.

🤖 Generated with Claude Code

@jlouvel jlouvel self-assigned this Oct 31, 2026
@jlouvel
jlouvel requested a balanced review from Copilot October 31, 2026 07:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A disconnect before the first body byte can hide truncated-response errors from synchronous callers, and the zero-length response test has a connection-pool race that can cause spurious timeouts.

2 open issues
What changed in this PR

This PR addresses #1601 by preserving synchronous HTTP response statuses when the Netty client fails after receiving the response head, building on the diagnostics added in #1602.

Changes:

  • Preserves synchronous response statuses on late failures and ends zero-length response streams immediately.
  • Identifies connection closures caused by client shutdown with "Client stopped".
  • Adds regression tests for late shutdown failures and empty-response connection reuse, plus a changelog entry.
File Description
org.restlet/​src/​test/​java/​org/​restlet/​engine/​connector/​ClientLateFailureTestCase.java Adds raw-socket tests for late failures and connection reuse.
org.restlet/​src/​main/​java/​org/​restlet/​engine/​netty/​NettyClientHelper.java Marks connections before closing them during shutdown.
org.restlet/​src/​main/​java/​org/​restlet/​engine/​netty/​NettyClientHandler.java Reports shutdown-specific failures, including through parent channels.
org.restlet/​src/​main/​java/​org/​restlet/​engine/​netty/​NettyClientCall.java Adjusts late-failure status handling and empty-body completion.
changes.md Documents the client response-handling fix.

Review effort: Balanced

@jlouvel
jlouvel force-pushed the fix/netty-client-late-failure-status branch 2 times, most recently from 5e636bb to f84a105 Compare November 1, 2026 16:18
jlouvel and others added 2 commits November 1, 2026 16:28
A failure after the response head no longer replaces the status returned to a
synchronous caller with a communication error (1001): it reaches the response
entity stream, and the onResponse callback of asynchronous requests. A
response announcing an empty body with "Content-Length: 0" is complete with
its head. The failures caused by stopping the client report "Client stopped"
instead of "Connection closed by peer".

This fixes the intermittent SecurityTestCase failures: the test released the
resource, stopping its client, before the end of the error page body.

Refs #1601

Co-Authored-By: Claude Opus 5.5 <[email protected]>
A synchronous caller resumes when the response body ends. Ending the body of
a response announcing an empty one at its head, or before releasing its
connection, let the caller send its next request before the connection was
back in the pool, so that request opened a new connection. This made
ClientLateFailureTestCase.completesResponsesWithAnEmptyBodyWithTheirHead time
out on CI. The body now ends once the response is complete and its connection
released, while failures after the head of an empty body are still ignored.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
When the connection failed after a response head announcing a body but
before its first byte, the entity was dropped, leaving a synchronous
caller with the status and no sign of the truncated response. The failed
stream is now kept as the entity, so that reading it reports the failure.
A response announcing an empty body still has no entity.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@sonarqubecloud

sonarqubecloud Bot commented Nov 1, 2026

Copy link
Copy Markdown

@jlouvel
jlouvel merged commit faf1676 into 3.0 Nov 1, 2026
3 checks passed
@jlouvel
jlouvel deleted the fix/netty-client-late-failure-status branch November 1, 2026 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants